Squeeze consecutive slashes in the path portion of the URI #228
Conversation
imtayadeway
force-pushed
the
squeeze-path-slashes
branch
from
88f6ca0
to
2e51d0a
Compare
There was a problem hiding this comment.
RFC 2396 defines a path parameter in a URL to be a single slash; The reason that multiple slashes are sometimes 'ok' is because historically URLs have pointed to files on disk and in most OS's multiple path separators in a row don't have any different affect. (ex: cd ./////manageiq will change the dir to manageiq in the current dir) So unless rewrite rules are in place, it 'usually works'.
With that knowledge, tbh I'd probably just say 'no, you're not making a proper request' and be done with it. If I were a client and making requests, I wouldn't be surprised if it didn't work if I accidentally added a slash, even as I acknowledge adding one on accident is really easy to do. cc/ @cben But as the change is a small one, let's discuss further...
It looks like Rails/Rack already does some squeezing...
Yup; ActionDispatch actually modifies the Rack request object, normalizing PATH_INFO here with this helper. As an external example, note you can add as many slashes as you want to the URL of this PR and GitHub (a Rails app) will still serve it to you correctly.
if there's a reason we're using the former over the latter, it's not obvious and it's not covered by tests.
The original_url is part of the ActionDispatch wrapper around Rack requests and generally is the correct way to get the actual URL requested in a Rails application, so not terribly surprising it's untested.
Given that the original version is used internally for things like http basic auth and the modified version is what's actually used in the router, I think this change is fine 👍 , unless there's some special auth parsing being done in the API I'm unaware of (I don't think this is the case though) cc/ @abellotti
fullpath instead of original_fullpath in the method at the bottom of this file, too
|
@chrisarcand for api auth, we rely on basic auth (HttpAuthentication::basic.authenticate ...) as well as all the other token based variants including external auth via request headers so the url does not come in play. Thanks. |
It looks like Rails/Rack already does some squeezing: ```ruby request.original_url # => "http://example.com//api" request.url # => "http://example.com/api" ``` If there's a reason we're using the former over the latter, it's not obvious and it's not covered by tests. Switching to the latter is sufficient to resolve the issue below. Closes ManageIQ#125
imtayadeway
force-pushed
the
squeeze-path-slashes
branch
from
2e51d0a
to
a72ccb9
Compare
|
@chrisarcand just rebased and incorporated suggestions. Thanks! |
|
Checked commit imtayadeway@a72ccb9 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 |
It looks like Rails/Rack already does some squeezing:
If there's a reason we're using the former over the latter, it's not
obvious and it's not covered by tests. Switching to the latter is
sufficient to resolve the issue below.
Closes #125